Created a custom rule for import-type-order #48

ismail-syed · 2017-03-10T14:02:18Z

Enforce a certain structure in your imports. Import structure should be listed in the following order:

Module imports
Absolute imports
Relative parent/ancestor directory imports
Relative sibling directory imports

Issue #42

ismail-syed · 2017-03-10T14:29:52Z

src/customRules/structuredImportsRule.ts

+  }
+}
+
+function getImportType(path: string): ImportType {


Any suggestions on making this look a little cleaner/prettier?

Eh, I'd go the other way and make it uglier. It's not going to expand beyond 4 possibilities, and most devs grok these hardcoded strings faster than pretty method names.

if (path.substr(1, 2) === './') { return Import.Sibling; } else if (path.substr(1, 4) === '../') {

If I was going to make it prettier, I'd try adding a static toType(path: string) function to the enum.

GoodForOneFare

Good start! Lots of comments, but nothing major.

GoodForOneFare · 2017-03-12T19:01:46Z

src/customRules/structuredImportsRule.ts

+  }
+
+  // e.g. 'import Foo from './foo';'
+  public visitImportDeclaration(node: ts.ImportDeclaration) {


So normally this would be cool, but I'm really concerned about performance. yarn check is taking over 2 minutes to run (>40% of CI time), and that'll only get worse as we add more code + rules. AFAIK, there's no way to parallelize tslint.

Taking a look at the tslint performance guide, it recommends:

Don’t walk the whole AST if possible

Implement your own walking algorithm

If you are interested in import statements for example, you only need to search in sourceFile.statements and nested NamespaceDeclaration

... and since nested declarations can't do module imports, that limits this rule to sourceFile.statements.

Here's a rough version of the above, with annotations on how to improve it. It passes for the existing tests, at least.

// This should be adapted to `extend Lint.AbstractWalker<MyOptionsType>`, and implement the `walk` rule. class StructuredImportsWalker extends Lint.RuleWalker { private previousImport: ImportType; private currentImport: ImportType; constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { super(sourceFile, options); } public visitSourceFile(node: ts.SourceFile) { const importNodes = node.statements .filter((child) => child.kind === ts.SyntaxKind.ImportDeclaration) .map((child) => child as ts.ImportDeclaration); if (importNodes.length === 0) { return; } // This is off the top of my head; probably a more readable way to do this. let lastGoodType = getImportType(importNodes.shift()); while (importNodes.length) { const currentImport = importNodes.shift(); const currentType = getImportType(currentImport); // Here we're just relying on enum comparisons. This gets rid of a couple of helper functions. if (lastGoodType > currentType) { this.addFailure( this.createFailure( currentImport.moduleSpecifier.getStart(), currentImport.moduleSpecifier.getFullWidth(), Rule.STRUCTURED_IMPORTS_ABS_FIRST_ERROR, ) ); } else { lastGoodType = currentType; } } } }

Thanks for the feedback!
I couldn't run my tests with this, earlier today. It needed a bit more refactoring but it was a great starting point.
Did you build then run the tests? I think that might of done it, it gets me all the time too.

Yep, I was using this to run the test:

yarn run build && $(npm bin)/tslint -r ./build/customRules/ --test test/rules/structured-imports/

The above code was a tweaked version of this ugly stuff (that definitely passes)... I'll hide this in a details block:

import * as ts from 'typescript'; import * as Lint from 'tslint';
export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys /
public static metadata: Lint.IRuleMetadata = {
'ruleName': 'structured-imports',
'description': 'Enforce structure to your imports. Import structure should be listed in the folloing order: modules, absolute imports, relative parent/ancestor directories, relative sibling directors.',
'hasFix': false,
'optionsDescription': 'Not configurable.',
'options': null,
'optionExamples': null,
'type': 'style',
'typescriptOnly': false,
};
/ tslint:enable:object-literal-sort-keys */

public static STRUCTURED_IMPORTS_ABS_FIRST_ERROR = 'Imports should be listed in the following order: module imports, absolute imports, relative imports.';

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
const structuredImportsWalker = new StructuredImportsWalker(sourceFile, this.getOptions());
return this.applyWithWalker(structuredImportsWalker);
}
}

enum ImportType {
Module = 0, // 'module'
Absolute = 1, // '/some/folder/file'
RelativeAncestor = 2, // '../parentFolder'
RelativeSibling = 3, // './siblingFolder'
};

const importStuctureOrder = [ImportType.Module, ImportType.Absolute, ImportType.RelativeAncestor, ImportType.RelativeSibling];

class StructuredImportsWalker extends Lint.RuleWalker {
private previousImport: ImportType;
private currentImport: ImportType;

constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
super(sourceFile, options);
}

public visitSourceFile(node: ts.SourceFile) {
const importNodes = node.statements
.filter((child) => child.kind === ts.SyntaxKind.ImportDeclaration)
.map((child) => child as ts.ImportDeclaration);

if (importNodes.length === 0) { return; } let lastGoodType = getImportType(importNodes.shift()); while (importNodes.length) { const currentImport = importNodes.shift(); const currentType = getImportType(currentImport); if (lastGoodType > currentType) { this.addFailure( this.createFailure( currentImport.moduleSpecifier.getStart(), currentImport.moduleSpecifier.getFullWidth(), Rule.STRUCTURED_IMPORTS_ABS_FIRST_ERROR, ) ); } else { lastGoodType = currentType; } }

}
}

function getImportType({moduleSpecifier}: ts.ImportDeclaration): ImportType {
const path = moduleSpecifier.getText();
if (isRelativeSibling(path)) {
return ImportType.RelativeSibling;
} else if (isRelativeAncestor(path)) {
return ImportType.RelativeAncestor;
} else if (isAbsolute(path)) {
return ImportType.Absolute;
}
return ImportType.Module;
}

function isAbsolute(path: string) {
return path[1] === '/' ; // [0] is quote mark
}

function isRelativeAncestor(path: string) {
return path.substr(1, 4) === '../' ; // path[0] is quote mark
}

function isRelativeSibling(path: string) {
return path.substr(1, 2) === './' ; // path[0] is quote mark
}

Okay, must of done something wrong when I was trying this our earlier today.

GoodForOneFare · 2017-03-12T19:05:33Z

docs/rules/structured-imports.md

+# Enforces a certain structure in your import statements
+
+## Rationale
+- Consistency in import statement structure throughout the codebase


This states a Shopify-specific end goal rather than a rationale. Looking through other rationales, adjacent-overload-signature has this:

Improves readability and organization by grouping naturally related items together.

That looks good to me.

GoodForOneFare · 2017-03-12T19:15:30Z

docs/rules/structured-imports.md

+- Consistency in import statement structure throughout the codebase
+
+## Rule Details
+Enforce a certain structure in your imports. Import structure should be listed in the following order:


Again, this is vague/flowery.

Imports should be in the following order: * Externals * Absolute paths * Parent directories * Siblings

Note that I'm borrowing "externals" from the eslint import/order rule. Everything in TypeScript is a module, so a more specific term is useful.

GoodForOneFare · 2017-03-12T19:17:30Z

docs/rules/structured-imports.md

+import BarModule from 'BarModule';
+```
+
+The following patterns are not warnings:


I'd prefer: The following import order is valid:

Sometimes coding rules apply to English! Less cognitive load to interpret "valid" than "not warnings".

GoodForOneFare · 2017-03-12T19:18:04Z

docs/rules/structured-imports.md

+
+## When Not To Use It
+
+If you do not with to enforce consistency in your import structuring.


Typo: with=>wish.

structuring is flowery; the sentence reads the same without it.

GoodForOneFare · 2017-03-12T19:56:57Z

test/rules/structured-imports/multiLine.ts.lint

@@ -0,0 +1,10 @@
+import Foo from './foo';


Again, I've no idea what multiLine is testing. This rule will never fail on a single import ¯\(ツ)/¯

GoodForOneFare · 2017-03-12T19:58:07Z

src/customRules/structuredImportsRule.ts

+    'options': null,
+    'optionExamples': null,
+    'type': 'style',
+    'typescriptOnly': false,


We don't care about JS, so let's make this true.

GoodForOneFare · 2017-03-12T19:58:36Z

src/customRules/structuredImportsRule.ts

+  /* tslint:disable:object-literal-sort-keys */
+  public static metadata: Lint.IRuleMetadata = {
+    'ruleName': 'structured-imports',
+    'description': 'Enforce structure to your imports. Import structure should be listed in the folloing order: modules, absolute imports,  relative parent/ancestor directories, relative sibling directors.',


Typo: folloing=>following.

GoodForOneFare · 2017-03-12T19:59:33Z

src/customRules/structuredImportsRule.ts

+enum ImportType {
+  Module = 0, // 'module'
+  Absolute = 1, // '/some/folder/file'
+  RelativeAncestor = 2, // '../parentFolder'


RelativeAncestor=>Ancestor (relative is implied).

GoodForOneFare · 2017-03-12T20:00:06Z

src/customRules/structuredImportsRule.ts

+  Module = 0, // 'module'
+  Absolute = 1, // '/some/folder/file'
+  RelativeAncestor = 2, // '../parentFolder'
+  RelativeSibling = 3, // './siblingFolder'


RelativeSibling=>Sibling.

GoodForOneFare · 2017-03-14T01:50:59Z

src/customRules/importTypeOrderRule.ts

+const importStuctureOrder = [ImportType.External, ImportType.Absolute, ImportType.Ancestor, ImportType.Sibling];
+
+class StructuredImportsWalker extends Lint.AbstractWalker<void>  {
+  private previousImport: ImportType;


What's the benefit of making the walker stateful?

Woah, that was fast!
There's no benefit to it being stateful. I was still refactoring, just pushed whatever I had that was in a 'happy state'.

Haha, right! Happy states are good practice. Sorry, too quick on the trigger 😛

No no, its okay. I appreciate the helpful comments more than you think!

ismail-syed · 2017-03-14T03:16:08Z

src/customRules/importTypeOrderRule.ts

@@ -57,7 +57,7 @@ function walker(context: Lint.WalkContext<void>): void {
 function getImportType(path: string): ImportType {
  if (path.substr(1, 2) === './') {
    return ImportType.Sibling;
-  } else if (path.substr(1, 4) === '../') {
+  } else if (path.substr(1, 3) === '../') {


The granular test cases are doing their job! 👍

ismail-syed · 2017-03-14T03:43:51Z

@GoodForOneFare All the changes are addressed. Have another look and let me know what you think.

I've created an issue for creating custom Rule fixes. I'll add those as separate PR's in the near future, so we can keep things moving with this PR.

GoodForOneFare

Just minor comments. With some quick cleanup, we'll be able to ship this today 🎉

GoodForOneFare · 2017-03-14T05:45:14Z

test/rules/structured-imports/valid-order.ts.lint

@@ -0,0 +1,4 @@
+import {Foo, Bar, Baz} from 'FooBarBaz';
+import BazTwo from '/Baz';
+import FooTwo from '../foo';


Can you add a side-effecty import somewhere in here, please?

GoodForOneFare · 2017-03-14T05:46:19Z

docs/rules/import-type-order.md

+- Improves readability and organization by grouping naturally related items together.
+
+## Rule Details
+Enforce order of import statement types. Import structure should be listed in the following order:


structure isn't adding anything to this sentence.

GoodForOneFare · 2017-03-14T05:47:23Z

docs/rules/import-type-order.md

+
+The following are considered warnings
+```js
+import Foo from './foo';


Please add some comments explaining the nature of each offence. e.g.,

import Foo from './foo'; // Should come after external and parent imports.

GoodForOneFare · 2017-03-14T05:48:21Z

src/customRules/importTypeOrderRule.ts

+enum ImportType {
+  External = 0, // 'external'
+  Absolute = 1, // '/some/folder/file'
+  Ancestor = 2, // '../parentFolder'


I'm still on the fence about Ancestor, but I can't think of anything better. Internal? Local? No. Bah.

Ohwell, they're not used outside this file, so we can easily change when inspiration strikes 😄

It's annoying because parents and ancestors are both valid imports for this type. I think Ancestor is the best name, given all our options.

GoodForOneFare · 2017-03-14T05:49:36Z

src/customRules/importTypeOrderRule.ts

+
+const importStuctureOrder = [ImportType.External, ImportType.Absolute, ImportType.Ancestor, ImportType.Sibling];
+
+function walker(context: Lint.WalkContext<void>): void {


GoodForOneFare · 2017-03-14T05:54:01Z

src/customRules/importTypeOrderRule.ts

+  }
+}
+
+function getImportType(path: string): ImportType {


Just accept getImportType({moduleSpecifier}: ts.ImportDeclaration), and fish out the text in here.

It's a minor reduction in the walker's verbosity, but every little bit helps.

GoodForOneFare · 2017-03-14T05:56:57Z

src/customRules/importTypeOrderRule.ts

+
+  let previousImport = getImportType(importNodes.shift().moduleSpecifier.getText());
+  while (importNodes.length) {
+    const currentImportNode = importNodes.shift().moduleSpecifier;


moduleSpecifier is of type StringLiteral, so this should be currentPath or currentModuleSpecifier.

GoodForOneFare · 2017-03-14T05:57:32Z

src/customRules/importTypeOrderRule.ts

+  let previousImport = getImportType(importNodes.shift().moduleSpecifier.getText());
+  while (importNodes.length) {
+    const currentImportNode = importNodes.shift().moduleSpecifier;
+    const currentImport = getImportType(currentImportNode.getText());


currentType would be more accurate.

GoodForOneFare · 2017-03-14T05:58:46Z

src/customRules/importTypeOrderRule.ts

+
+  if (importNodes.length === 0) { return; }
+
+  let previousImport = getImportType(importNodes.shift().moduleSpecifier.getText());


previous implies that the loop is just dumbly assigning to this var, but it's only assigning types for valid imports.

... and it's not an import. It's a lastValidType.

GoodForOneFare · 2017-03-14T06:03:28Z

A CHANGELOG update would be nice, too.

ismail-syed · 2017-03-14T14:03:18Z

@GoodForOneFare I think we're good now! 🤞

ismail-syed · 2017-03-14T18:23:12Z

Ewww. Thought I'd give the GitHub web ui a shot for fixing merge conflicts.
It left a merge commit Merge branch 'master' into custom-rule_structured-imports.
Pretty convenient though.

GoodForOneFare

Fix more pedantry and 🚢 👍

GoodForOneFare · 2017-03-14T21:35:46Z

docs/rules/import-type-order.md

+
+```js
+export interface TheThreeStooges {
+import BarModule from 'BarModule';


imports should be indented.

Wow, I don't even know why I wrapped those imports in an interface. Probably pulled code from somewhere and forgot to get rid of the export.

GoodForOneFare · 2017-03-14T21:38:26Z

src/customRules/importTypeOrderRule.ts

+  /* tslint:disable:object-literal-sort-keys */
+  public static metadata: Lint.IRuleMetadata = {
+    'ruleName': 'import-type-order',
+    'description': 'Enforce structure to your imports. Import structure should be listed in the following order: modules, absolute imports,  relative parent/ancestor directories, relative sibling directors.',


Improves readability and organization by grouping related imports together. Imports should be listed in order of: external modules, absolute paths, relative paths, relative siblings.

This should've been fixed.

GoodForOneFare · 2017-03-14T21:39:11Z

src/customRules/importTypeOrderRule.ts

+  };
+  /* tslint:enable:object-literal-sort-keys */
+
+  public static IMPORT_TYPE_ORDER_ERROR = 'Imports should be listed in the following order: module imports, absolute imports, ancestor imports, sibling imports.';


s/module/external

ismail-syed force-pushed the custom-rule_structured-imports branch from 26a470b to 73d9b7a Compare March 10, 2017 14:03

ismail-syed self-assigned this Mar 10, 2017

ismail-syed requested a review from GoodForOneFare March 10, 2017 14:13

ismail-syed commented Mar 10, 2017

View reviewed changes

GoodForOneFare suggested changes Mar 12, 2017

View reviewed changes

GoodForOneFare reviewed Mar 14, 2017

View reviewed changes

ismail-syed force-pushed the custom-rule_structured-imports branch from 3d82bcf to 408a9a0 Compare March 14, 2017 02:13

ismail-syed commented Mar 14, 2017

View reviewed changes

ismail-syed changed the title ~~Created a custom rule for structured-imports~~ Created a custom rule for import-type-order Mar 14, 2017

ismail-syed mentioned this pull request Mar 14, 2017

Create fixes for custom rules #51

Open

GoodForOneFare suggested changes Mar 14, 2017

View reviewed changes

GoodForOneFare approved these changes Mar 14, 2017

View reviewed changes

ismail-syed force-pushed the custom-rule_structured-imports branch from 0751d80 to f1a2311 Compare March 15, 2017 02:06

ismail-syed added 13 commits March 14, 2017 22:07

Added structured-imports rule ✨

44766ae

Clean up and tweaks

8553145

Add rule docs 📝

1923877

Change error messages

0033fc8

Fix tlint error 🔨

cac50df

Enable custom rule

bf31490

PR fixes 🔨

5e0a6ec

Clean up getImportType()

16aa5f6

Clean error wording and use footnote definition in test cases

81f58fd

Refactor rule for minor performance improvements with AbstractWalker

a737379

Utilize applyWithFunction() instead of applyWithWalker()

46c850e

Add more speicific test cases

e5610a4

Add test case side-effects imports

2c125da

ismail-syed added 3 commits March 14, 2017 22:07

PR Fixes, round 2 🔨

f9c4c4b

Update CHANGELOG.md 📝

2f585aa

PR fixes, round 3 🔨

97f6669

ismail-syed force-pushed the custom-rule_structured-imports branch from f1a2311 to 97f6669 Compare March 15, 2017 02:08

Fix test cases

8bbccdc

ismail-syed merged commit 60ec7b8 into master Mar 15, 2017

ismail-syed mentioned this pull request Mar 15, 2017

Fix docs on import-type-order #53

Merged


		## When Not To Use It

		If you do not with to enforce consistency in your import structuring.


		const importStuctureOrder = [ImportType.External, ImportType.Absolute, ImportType.Ancestor, ImportType.Sibling];

		function walker(context: Lint.WalkContext<void>): void {


		if (importNodes.length === 0) { return; }

		let previousImport = getImportType(importNodes.shift().moduleSpecifier.getText());

Created a custom rule for import-type-order #48

Created a custom rule for import-type-order #48

Conversation

ismail-syed commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoodForOneFare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed commented Mar 14, 2017

GoodForOneFare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoodForOneFare commented Mar 14, 2017

ismail-syed commented Mar 14, 2017

ismail-syed commented Mar 14, 2017 • edited Loading

GoodForOneFare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed commented Mar 10, 2017 •

edited

Loading

ismail-syed commented Mar 14, 2017 •

edited

Loading